Skip to content

lsp worktree init #576

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

lsp worktree init #576

wants to merge 3 commits into from

Conversation

@Saga4
Copy link
Contributor

@Saga4 Saga4 commented Jul 24, 2025 •
edited by github-actions bot
Loading

PR Type

Enhancement

Description

  • Add LSP commands for git worktree management

  • Introduce WorktreeParams dataclass for requests

  • Implement createWorktree to setup worktrees

  • Implement removeWorktree to cleanup worktrees

Diagram Walkthrough

flowchart LR
  A["VSCode LSP request"] -- "createWorktree" --> B["create_worktree handler"]
  B --> C["create_worktree_root_dir"]
  B --> D["create_git_worktrees"]
  D --> E["register worktrees"]
  E --> F["return success response"]
  A -- "removeWorktree" --> G["remove_worktree handler"]
  G --> H["remove_git_worktrees"]
  H --> I["cleanup registry"]
  I --> J["return success response"]
Loading

File Walkthrough
Relevant files
Enhancement
beta.py
Add worktree management functions                                               

codeflash/lsp/beta.py

  • Imported git utility functions: create_git_worktrees,
    create_worktree_root_dir, remove_git_worktrees
  • Added WorktreeParams dataclass for LSP params
  • Implemented createWorktree LSP feature for creating worktrees
  • Implemented removeWorktree LSP feature for cleanup
 +114/-0 

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Input Validation

The gitRoot, functionName, and candidateId parameters are used directly for filesystem and registry operations without sanitization. Consider validating and sanitizing inputs to avoid path traversal or injection risks.

 
try:
    module_root = Path(params.gitRoot)

    # Create worktree root directory
    git_root, worktree_root_dir = create_worktree_root_dir(module_root)
Race Condition

The shared server.worktree_registry is modified without synchronization. Concurrent LSP requests could cause inconsistent state. Consider adding thread-safety or locking mechanisms.

 
if not hasattr(server, "worktree_registry"):
    server.worktree_registry = {}

server.worktree_registry[params.candidateId] = {
    "worktree_root": worktree_root,
    "worktrees": worktrees,
    "function_name": params.functionName,
}
Broad Exception

Both handlers catch all exceptions, potentially hiding unexpected errors and making debugging harder. It's better to catch specific exceptions and let others surface.

 
except Exception as e:
    server.show_message_log(f"Error creating worktree: {e!s}", "Error")
    return {
        "functionName": params.functionName,
        "candidateId": params.candidateId,
        "status": "error",
        "message": f"Error creating worktree: {e!s}",
    }

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate gitRoot directory

Validate that params.gitRoot points to an existing directory before proceeding to
avoid confusing failures when the path is invalid. If the check fails, log an error
and return an appropriate error response.

codeflash/lsp/beta.py [313]

 module_root = Path(params.gitRoot)
+if not module_root.is_dir():
+    server.show_message_log("Invalid gitRoot path", "Error")
+    return {
+        "functionName": params.functionName,
+        "candidateId": params.candidateId,
+        "status": "error",
+        "message": "Invalid gitRoot path",
+    }
Suggestion importance[1-10]: 6

__

Why: Adding a directory existence check on module_root prevents proceeding with an invalid gitRoot and avoids downstream errors.

Low
General
Use pop to remove entry

Replace the del statement with pop to remove the entry and safely handle missing
keys without raising a KeyError.

codeflash/lsp/beta.py [394]

-del server.worktree_registry[params.candidateId]
+server.worktree_registry.pop(params.candidateId, None)
Suggestion importance[1-10]: 5

__

Why: Replacing del with pop safely handles missing keys without raising a KeyError and improves error resilience.

Low
Initialize registry atomically

Use getattr to initialize worktree_registry with a default empty dict in one line,
preserving any existing entries and avoiding repeated attribute checks.

codeflash/lsp/beta.py [340-341]

-if not hasattr(server, "worktree_registry"):
-    server.worktree_registry = {}
+server.worktree_registry = getattr(server, "worktree_registry", {})
Suggestion importance[1-10]: 4

__

Why: The getattr approach reduces boilerplate for initializing worktree_registry, but it’s a minor style improvement.

Low

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

saga4 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Saga4 Saga4 requested a review from mohammedahmed18 August 10, 2025 19:26
mohammedahmed18
mohammedahmed18 requested changes Aug 12, 2025
View reviewed changes
codeflash/lsp/beta.py
Comment on lines +260 to +266
function_optimizer = server.optimizer.create_function_optimizer(
current_function,
function_to_optimize_source_code=validated_original_code[current_function.file_path].source_code,
original_module_ast=original_module_ast,
original_module_path=current_function.file_path,
function_to_tests=server.optimizer.discovered_tests or {},
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the function_optimizer here is pointing to the same path (current_function.file_path)
not the worktree path.

codeflash/lsp/beta.py
# Create worktrees for optimization candidates if in git repo
server.show_message_log(f"Creating worktrees for optimization candidates of: {params.functionName}", "Info")
git_root, worktree_root_dir = None, None
worktree_root, worktrees = None, []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have multiple worktrees here ?

@Saga4 Saga4 closed this Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mohammedahmed18 mohammedahmed18 mohammedahmed18 requested changes

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Saga4 @CLAassistant @mohammedahmed18